Skip to content

fixed Sidebar theme#1979

Merged
Luke9389 merged 1 commit into
Expensify:masterfrom
parasharrajat:parasharrajat/Screenwrapper
Mar 23, 2021
Merged

fixed Sidebar theme#1979
Luke9389 merged 1 commit into
Expensify:masterfrom
parasharrajat:parasharrajat/Screenwrapper

Conversation

@parasharrajat

@parasharrajat parasharrajat commented Mar 22, 2021

Copy link
Copy Markdown
Member

Please review @marcaaron. As these changes are directly linked to your code.

Details

<SafeAreaInsetsContext.Consumer> does not have styling support but in ScreenWrapper component was passing parent style prop to it. I have moved it to the child View which is working without regression.

Fixed Issues

Fixes #1967

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android
Before After
image image

CC @shawnborton

@parasharrajat parasharrajat requested a review from a team as a code owner March 22, 2021 16:22
@botify botify requested review from Luke9389 and removed request for a team March 22, 2021 16:22
@parasharrajat parasharrajat changed the title Sidebar Theme is not correct. Sidebar theme fixed Mar 22, 2021
@parasharrajat parasharrajat changed the title Sidebar theme fixed fixed Sidebar theme Mar 22, 2021
@shawnborton

Copy link
Copy Markdown
Contributor

Looks good but would love to see screenshots of all of the platforms, especially iOS.

Comment thread src/components/ScreenWrapper.js Outdated
@parasharrajat parasharrajat force-pushed the parasharrajat/Screenwrapper branch from c1e9571 to 94f7b14 Compare March 22, 2021 19:51
@parasharrajat

Copy link
Copy Markdown
Member Author

@shawnborton Unfortunately, I don't have IOS. Here are screenshots for other platforms. As per your query, the Statusbar is transparent and its color depends on the screen below.

Desktop

Screenshot_20210323_005459

Mobile Web

Screenshot_2021-03-23 Expensify cash - Chat with friends to send and receive money

Android

Screenshot_2021-03-23-01-16-29-250_com expensify chat

@parasharrajat parasharrajat requested a review from Luke9389 March 22, 2021 19:59
@parasharrajat parasharrajat force-pushed the parasharrajat/Screenwrapper branch 3 times, most recently from ff32c17 to 83b16d7 Compare March 22, 2021 20:12
Comment thread src/components/ScreenWrapper.js Outdated
@parasharrajat parasharrajat force-pushed the parasharrajat/Screenwrapper branch from 83b16d7 to e8734d1 Compare March 23, 2021 14:31
@parasharrajat

Copy link
Copy Markdown
Member Author

@Luke9389 Updated.
@shawnborton Any comments on the screenshots above.
Thanks.

@shawnborton

Copy link
Copy Markdown
Contributor

Let me pull this down and test on iOS for you.

@shawnborton

Copy link
Copy Markdown
Contributor

Looks good on iOS!

@Luke9389

Copy link
Copy Markdown
Contributor

@parasharrajat nice work on this PR! Sorry that my reviews may have felt a bit... nit-picky. In the future, please know that if I ask a question on a review, I really don't know the answer. So don't feel like I'm asking you rhetorical questions, or that I'm hinting at something. I'll always be direct 😄. Anyway, good job, and thank you! 🙇‍♂️

@Luke9389 Luke9389 merged commit f93683f into Expensify:master Mar 23, 2021
@parasharrajat

Copy link
Copy Markdown
Member Author

@Luke9389. I actually like the nit-picky reviews. it helps in decision-making. Eventually, a review is a kind of second personality's point of view. So tackling all these reviews is making me good at making decisions related to code. I am sure it will help in the long run when we have to work over a lib for developers and not for consumers. 👍

@laurenreidexpensify

Copy link
Copy Markdown
Contributor

@parasharrajat I will make a plan to get you paid for this PR as a bonus on your next Upwork payment 👍🏽

@parasharrajat

Copy link
Copy Markdown
Member Author

@laurenreidexpensify Amazing. Thanks

@laurenreidexpensify

Copy link
Copy Markdown
Contributor

Okay as that was merged on Tues 23 March, will pay you on 30 March as per the 7 day regression test 👍🏽

@parasharrajat parasharrajat deleted the parasharrajat/Screenwrapper branch November 4, 2022 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LHN background color is not correct.

4 participants